feat: dynamic gas price, keeper implementation #2838

piux2 · 2024-09-24T09:20:37Z

Context

This PR is inspired by EIP-1559 and adjusts the gas price based on the ratio of gas used in the last block compared to the target block gas. The gas price is enforced globally across the network. However, validators can still configure a minimum gas price (min-gas-price) to reject low-fee transactions and prevent mempool spam. A higher gas price will take precedence when configured.

Current implementation is an alternative to PR2544 and is based on the feedbacks.

Here are the main differences:

Dynamic gas prices are managed by a new auth.GasPriceKeeper, rather than being saved in the block header.
Gas price configurations have been moved from consensus parameters to GnoGenesisState and are stored in a new parameter module.
The parameters can be modified later through governance proposals, making it easier to update these configurations without requiring a chain upgrade.
All implementations are on the application side, with no changes made to the consensus layer.

High level flow

Start a new node from genesis. The initial gas price and formula parameters are saved in the genesis and loaded into the params keeper and gas keeper.

When a node receives a new transaction, the application checks if the user has provided sufficient fees for the transaction. It will reject the transaction if it does not meet the gas price set by the network and individual nodes.

The node processes the entire block during the proposal, voting, and restart phases.
The GasPriceKeeper will calculate and update the gas price according to the formula in the application’s EndBlock() function.

Formular

The compressor is used to reduce the impact on price caused by sudden changes in the gas used within a block

When the last gas used in a block is above the target gas, we increase the gas price

When the last gas used in a block is below the target gas, we decrease the gas price until it returns to the initial gas price in the block.

Impact

The Cosmos SDK has an optional setting for a minimum gas price. Each validator can configure their own values to only accept transactions with a gas price that meets their setting in the mempool. When a user submits a transaction on-chain, the gas price is calculated as gas-fee / gas-wanted.

With the addition of the block gas price, a network-wide minimum gas price is enforced for every validator. Users will need to provide a gas price that meets the requirements set by both the validator and the network.

Contributors' checklist...

Added new tests
Provided an example (e.g. screenshot) to aid review
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

codecov · 2024-09-24T09:21:52Z

Codecov Report

Attention: Patch coverage is 58.51852% with 112 lines in your changes missing coverage. Please review.

Project coverage is 60.98%. Comparing base (9897b66) to head (5d6be56).
Report is 44 commits behind head on master.

Files with missing lines	Patch %	Lines
tm2/pkg/sdk/auth/keeper.go	46.66%	35 Missing and 5 partials ⚠️
tm2/pkg/sdk/auth/params.go	41.86%	17 Missing and 8 partials ⚠️
tm2/pkg/sdk/params/keeper.go	60.00%	6 Missing and 4 partials ⚠️
tm2/pkg/sdk/auth/genesis.go	0.00%	6 Missing ⚠️
tm2/pkg/sdk/auth/types.go	0.00%	6 Missing ⚠️
tm2/pkg/telemetry/metrics/metrics.go	0.00%	6 Missing ⚠️
gno.land/pkg/integration/testing_node.go	60.00%	4 Missing ⚠️
tm2/pkg/sdk/auth/abci.go	0.00%	4 Missing ⚠️
tm2/pkg/sdk/baseapp.go	33.33%	3 Missing and 1 partial ⚠️
gno.land/pkg/gnoland/app.go	88.88%	1 Missing and 1 partial ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2838      +/-   ##
==========================================
+ Coverage   60.87%   60.98%   +0.11%     
==========================================
  Files         563      568       +5     
  Lines       75193    75606     +413     
==========================================
+ Hits        45770    46111     +341     
- Misses      26055    26085      +30     
- Partials     3368     3410      +42

Flag	Coverage Δ
contribs/gnodev	`60.71% <100.00%> (-0.75%)`	⬇️
contribs/gnofaucet	`14.46% <ø> (-0.86%)`	⬇️
gno.land	`67.99% <81.63%> (+0.81%)`	⬆️
gnovm	`65.78% <ø> (+0.14%)`	⬆️
misc/genstd	`80.54% <ø> (ø)`
misc/logos	`19.88% <ø> (-0.36%)`	⬇️
tm2	`62.00% <49.75%> (-0.09%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

deelawn

Here are some initial questions and comments per our conversation earlier.

tm2/pkg/std/gasprice.go

gno.land/pkg/gnoland/app.go

deelawn · 2024-10-07T23:38:47Z

gno.land/pkg/gnoland/app.go

@@ -107,9 +110,11 @@ func NewAppWithOptions(cfg *AppOptions) (abci.Application, error) {
 		func(ctx sdk.Context, tx std.Tx, simulate bool) (
 			newCtx sdk.Context, res sdk.Result, abort bool,
 		) {
+			// Add last gas price in the context
+			ctx = ctx.WithValue(auth.GasPriceContextKey{}, gpKpr.LastGasPrice(ctx))


Maybe a refactor is necessary generally, for this and other key-value pairs being put in the std.Context instance, but it seems like data that is always necessary should be part of the Context struct's definition. This approach still makes sense for storing key-value pairs for module data that is not required.

this approach still makes sense for storing key-value pairs for module data that is not required.

Good point. We can address the refactor in a separate PR later

deelawn · 2024-10-07T23:40:51Z

gno.land/pkg/gnoland/genesis.go

+
+func DefaultGenState() GnoGenesisState {
+	authGen := auth.DefaultGenesisState()
+	gp, err := std.ParseGasPrice(InitGasPrice)


Can InitGasPrice be a GasPrice struct so it doesn't need to be parsed?

InitGasPrice is a default genesis value declared as a constant. Go only allows primitive types for constants.

I agree with Dylan, it doesn't make sense to parse it every time, when we can just declare it as a global var

zivkovicmilos

I think the PR looks alright 💯

The way we transfer params, and load latest gas values through keepers in different modules is not a pattern we should be using, but I realize that the alternatives require much, much chunkier changes to how modules communicate.

There needs to be some heavy rebasing done with master, but I haven't seen anything blocking in the PR. We should be good

zivkovicmilos · 2024-11-19T06:08:26Z

contribs/gnodev/go.mod

@@ -1,6 +1,6 @@
 module github.com/gnolang/gno/contribs/gnodev

-go 1.22
+go 1.22.0


I think we can skip the minor version

Yes, it is to fix the issue of being unable to download the toolchain. I just realized that adding toolchain go1.22.4 is a better solution.

zivkovicmilos · 2024-11-19T06:11:28Z

tm2/pkg/telemetry/metrics/metrics.go

@@ -104,6 +105,9 @@ var (
 	// BlockSizeBytes measures the size of the latest block in bytes
 	BlockSizeBytes metric.Int64Histogram

+	// BlockGasPriceAmount measures the block gas price of the last block
+	BlockGasPriceAmount metric.Int64Histogram


Good call on also adding this as a metric :)

zivkovicmilos · 2024-11-19T06:12:08Z

tm2/pkg/std/gasprice.go

@@ -48,3 +53,28 @@ func ParseGasPrices(gasprices string) (res []GasPrice, err error) {
 	}
 	return res, nil
 }
+
+// IsGTE compares the GasPrice with another gas price B. If coin denom matches AND fee per gas is
+// greater or equal to the gas price B return true, other wise return false,


Typo here, otherwise

zivkovicmilos · 2024-11-19T06:12:43Z

tm2/pkg/std/gasprice.go

+// greater or equal to the gas price B return true, other wise return false,
+func (gp GasPrice) IsGTE(gpB GasPrice) bool {
+	if gp.Price.Denom != gpB.Price.Denom {
+		panic(fmt.Sprintf("gas price denominations should be equal; %s, %s", gp.Price.Denom, gpB.Price.Denom))


Do we want the panics here? Where are they caught?

The Gas price is compared in sdk/ante handler. The panic is caught in sdk/base app for all transactions.
However, I think it would be better to return error instead of paniking, as this involves user input.
I will fix it.

zivkovicmilos · 2024-11-19T06:18:49Z

tm2/pkg/std/gasprice.go

+	prod1 := big.NewInt(0).Mul(gpa, gpBg) // gp's price amount * gpB's gas
+	prod2 := big.NewInt(0).Mul(gpg, gpBa) // gpB's gas * pg's price amount
+	// This is equivalent to checking
+	// That the Fee / GasWanted ratio is greater than or equal to the minimum GasPrice per gas.


zivkovicmilos · 2024-11-19T07:05:23Z

tm2/pkg/sdk/auth/keeper.go

+		num.Div(num, targetGasInt)
+		num.Div(num, denom.SetInt64(c))
+		// increase at least 1
+		diff := max(num, big.NewInt(1))


Maybe it makes sense to optimize this a bit by defining a common var bigOne = big.NewInt(1)

zivkovicmilos · 2024-11-19T07:05:47Z

tm2/pkg/sdk/auth/keeper.go

+
+	// targetGas = maxGax*TargetGasRatio/100
+
+	num.Mul(big.NewInt(maxGas), big.NewInt(params.TargetGasRatio))


Have you thought about using holiman's uint256, instead of big.Int?

zivkovicmilos · 2024-11-19T07:06:45Z

tm2/pkg/sdk/auth/keeper.go

+		num = max(num, initPriceInt)
+	}
+
+	if !num.IsInt64() {


When is this possible?

If it is, why don't we just set the biggest possible gas price that's relevant?

zivkovicmilos · 2024-11-19T07:07:06Z

tm2/pkg/sdk/auth/keeper.go

+}
+
+// max returns the larger of x or y.
+func max(x, y *big.Int) *big.Int {


Nitpick: maybe consider naming this maxBig

zivkovicmilos · 2024-11-19T07:10:40Z

gno.land/pkg/gnoland/types.go

-	Txs      []std.Tx  `json:"txs"`
+	Balances []Balance         `json:"balances"`
+	Txs      []std.Tx          `json:"txs"`
+	Auth     auth.GenesisState `json:"auth"`


This has been added as part of Manfred's recent PR to master

feat: dynamic gas price, keeper implementation

cb8cc2f

piux2 requested review from gfanton, moul, thehowl, zivkovicmilos, leohhhn, a team and jaekwon as code owners September 24, 2024 09:20

github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Sep 24, 2024

github-actions bot assigned piux2 Sep 24, 2024

piux2 added 2 commits September 24, 2024 03:03

added new files

e7cf296

fix dev node genesis AppState

7696e58

Kouteki mentioned this pull request Oct 4, 2024

Minutes: Core Staff Weekly Syncs [every Monday] gnolang/meetings#36

Open

bug fix: textar non-validator restart failure

ae25c58

deelawn reviewed Oct 7, 2024

View reviewed changes

piux2 added 4 commits October 10, 2024 17:48

more testing coverage

6da36bb

more test

6f89b6b

export methods

8b71aa3

more tests

5d6be56

Kouteki added the in focus Core team is prioritizing this work label Oct 18, 2024

zivkovicmilos added this to the 🚀 Mainnet launch milestone Oct 28, 2024

Kouteki removed request for a team and leohhhn November 1, 2024 16:39

more param test

1504d8f

Kouteki removed request for gfanton, moul, thehowl and jaekwon November 15, 2024 09:42

This was referenced Nov 18, 2024

[chain] Gas Estimations #1826

Open

[WIP] Test6 launch #3142

Open

zivkovicmilos approved these changes Nov 19, 2024

View reviewed changes

This was referenced Nov 27, 2024

"Gas used" seems unrelated to the number of coins subtracted #1070

Open

Bug: contract execution fee is added on top of the transaction fee. #1067

Closed

Refactor gasprice.IsGTE to return an error instead of causing a panic.

53dfbd3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dynamic gas price, keeper implementation #2838

feat: dynamic gas price, keeper implementation #2838

piux2 commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

deelawn left a comment

deelawn Oct 7, 2024

piux2 Oct 11, 2024 •

edited

Loading

deelawn Oct 7, 2024

piux2 Oct 11, 2024 •

edited

Loading

zivkovicmilos Nov 19, 2024

zivkovicmilos left a comment

zivkovicmilos Nov 19, 2024

piux2 Nov 27, 2024 •

edited

Loading

zivkovicmilos Nov 19, 2024

zivkovicmilos Nov 19, 2024

zivkovicmilos Nov 19, 2024

piux2 Nov 27, 2024

zivkovicmilos Nov 19, 2024

zivkovicmilos Nov 19, 2024

zivkovicmilos Nov 19, 2024 •

edited

Loading

zivkovicmilos Nov 19, 2024

zivkovicmilos Nov 19, 2024

zivkovicmilos Nov 19, 2024


		// targetGas = maxGax*TargetGasRatio/100

		num.Mul(big.NewInt(maxGas), big.NewInt(params.TargetGasRatio))

feat: dynamic gas price, keeper implementation #2838

Are you sure you want to change the base?

feat: dynamic gas price, keeper implementation #2838

Conversation

piux2 commented Sep 24, 2024 • edited Loading

Context

High level flow

Formular

Impact

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

deelawn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piux2 Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piux2 Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivkovicmilos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piux2 Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivkovicmilos Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piux2 commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

piux2 Oct 11, 2024 •

edited

Loading

piux2 Oct 11, 2024 •

edited

Loading

piux2 Nov 27, 2024 •

edited

Loading

zivkovicmilos Nov 19, 2024 •

edited

Loading